Fix truncation and noDegenerate bypasses in wc_PKCS7_VerifySignedData - #11017
Fix truncation and noDegenerate bypasses in wc_PKCS7_VerifySignedData#11017miyazakh wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes two PKCS#7 SignedData verification bypasses in wc_PKCS7_VerifySignedData() that could otherwise return success and populate pkcs7->content from unauthenticated data, and adds regression tests covering both bypass scenarios (streaming and non-streaming builds).
Changes:
- Enforce
noDegeneratewhensignerInfosis truly degenerate (empty) even ifdigestAlgorithmsis non-empty. - Remove the streaming stage-3 “< 6 bytes remaining” success early-exit by bounding lookahead to the bytes actually remaining.
- Add new API tests for minimal valid degenerate bundles and for truncated/missing
signerInfoscases; broaden an existing truncation test to run in both build modes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wolfcrypt/src/pkcs7.c | Tightens degenerate/noDegenerate enforcement and adjusts streaming footer lookahead to prevent truncation being treated as successful completion. |
| tests/api/test_pkcs7.h | Adds declarations/registration for new SignedData verification regression tests. |
| tests/api/test_pkcs7.c | Adds new regression tests for minimal degenerate success, truncation rejection, and non-empty digestAlgorithms + empty signerInfos rejection; updates an existing truncation test gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hoist ret/idx declarations to the top of the HAVE_PKCS7 block instead of
a standalone { ... } scope, per wolfSSL C coding style.
Stage 3's cap on stream->expected only applied while totalRd < maxLen. When a bundle is truncated exactly at the outer SignedData length (0 bytes remain, totalRd == maxLen), the cap did not fire, leaving expected at a fixed non-zero value. Stage 4 then requested bytes that would never arrive, returning WC_PKCS7_WANT_READ_E instead of a deterministic parse error, in both single-shot and streaming calls. Cap expected to 0 when totalRd >= maxLen so stage 4's existing bounds check fires immediately with BUFFER_E. Strengthen test_wc_PKCS7_VerifySignedData_NoSignerInfosTag to assert the return code is not WC_PKCS7_WANT_READ_E, and add a streaming byte-at-a-time variant, since the previous ExpectIntNE(ret, 0) check did not catch the stall.
The totalRd >= maxLen zero-cap added in 8777c2b also fired for BER indefinite-length bundles, where maxLen is only a running estimate set from the first available bytes and grows as parsing progresses, not a hard bound on the message size. That made totalRd catch up to maxLen mid-stream on legitimate encode/verify round trips, turning a normal WC_PKCS7_WANT_READ_E into a premature BUFFER_E/ASN_PARSE_E and breaking --enable-all make check (test_wc_PKCS7_EncodeSignedData, test_wc_PKCS7_VerifySignedData_RSA, test_wc_PKCS7_BER). Only apply the zero-cap when pkcs7->stream->indefLen is not set, so it's limited to definite-length bundles where maxLen is authoritative.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11017
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
retest this please |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 10 total — 10 posted, 0 skipped
9 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Critical] Stage 3 fall-through breaks valid degenerate bundles at nearly every chunk size (permanent WC_PKCS7_WANT_READ_E stall) —
wolfcrypt/src/pkcs7.c:7509-7535 - [High] Stage 3 'bytes remaining' cap omits + stream-length, diverging chunked from single-shot results on valid input —
wolfcrypt/src/pkcs7.c:7519-7535 - [High] TruncSignerInfosTag asserts on a double-call artifact, masking that the PR's stated goal is not met for this vector —
tests/api/test_pkcs7.c:6468-6470 - [Medium] New tests only cover chunk=1 and single-shot, which is why the stage 3 regression passed CI —
tests/api/test_pkcs7.c:6404-6423 - [Medium] No regression test that noDegenerate=1 still ACCEPTS a bundle with a real signer —
tests/api/test_pkcs7.c:6606-6608 - [Medium] Streaming loops never assert that all bytes were consumed, so an early success is indistinguishable from a full parse —
tests/api/test_pkcs7.c:6410-6417 - [Low] NoSignerInfosTag repeats the double-call idiom; currently self-consistent but fragile —
tests/api/test_pkcs7.c:6521-6523 - [Low] Stage 6 zero-cap comment describes a stage 7 bounds check that does not exist —
wolfcrypt/src/pkcs7.c:8132-8143 - [Low] Bare PKCS7_NO_SIGNER_E instead of WC_NO_ERR_TRACE(PKCS7_NO_SIGNER_E) —
tests/api/test_pkcs7.c:6607-6608
Findings not tied to a diff line
TruncSignerInfosTag and DegenerateNonEmptyDigestAlgos have no streaming sub-test, unlike their siblings
File: tests/api/test_pkcs7.c:6435,6559
Function: test_wc_PKCS7_VerifySignedData_TruncSignerInfosTag
Severity: Medium
The PR's justification for these tests is the streaming stage 3/4/7 rework, yet only two of the four new tests exercise the streaming path. DegenerateMinimal (6404-6423) and NoSignerInfosTag (6527-6543) add a #ifndef NO_PKCS7_STREAM one-byte-at-a-time loop; TruncSignerInfosTag (6435) and DegenerateNonEmptyDigestAlgos (6559) do not.
The single-shot path is not equivalent: for TruncSignerInfosTag the single-shot call returns -270 (WANT_READ_E) while the byte-at-a-time loop returns -141 after consuming 100/101 bytes -- two entirely different code paths with different outcomes, and only the weaker one is covered. Both missing loops work when added (measured: TruncSignerInfosTag bytewise -> -141; DegenerateNonEmptyDigestAlgos bytewise -> -269 PKCS7_NO_SIGNER_E).
Related gaps: none of the four new blobs carries a certificates [0] SET or a CRLs [1] SET, so the stage 3 -> 4 transition with real optional footers is untested by any new test -- which is exactly the shape that breaks (see BLOCK finding 2). No new test covers an indefinite-length (BER) bundle even though this PR added an explicit indefLen carve-out at stage 3 in commit 59c4c7087; that path is only covered incidentally by the pre-existing test_wc_PKCS7_BER.
Recommendation: Also add (a) a degenerate bundle carrying a real certificates [0] SET and a CRLs [1] SET, and (b) an indefinite-length/BER variant, to pin the indefLen carve-out this PR introduces.
Referenced code: tests/api/test_pkcs7.c:6435,6559 (8 lines)
Review generated by Skoll
one more
Summary
wc_PKCS7_VerifySignedData()had two bugs that let signature verification be silently bypassed (F-7302). Both returned0(success) withpkcs7->contentpopulated from unauthenticated data.NO_PKCS7_STREAMundefined): a bundle truncated 0-5 bytes short of a completesignerInfosSET was accepted as a valid degenerate (certs-only) end.(f7302)digestAlgorithmsand emptysignerInfosbypassedwc_PKCS7_AllowDegenerate(pkcs7, 0)entirely.Bug 1: truncation accepted as a valid degenerate end
WC_PKCS7_VERIFY_STAGE3treated any input with fewer than 6 bytes remaining after the content as a finished, successful parse, unable to distinguish a genuine minimal degenerate end (31 00) from a truncated bundle. Dates back to3cad0bfe5, which left the gap with an acknowledging comment.Fix: stage 3 no longer returns early. It caps
pkcs7->stream->expectedto the bytes actually remaining, then lets stage 4/5/6's existing bounds checks run normally, so truncated input fails there instead of being silently accepted.Bug 2: noDegenerate bypass via non-empty digestAlgorithms
wc_PKCS7_ParseSignerInfo()only enforcednoDegeneratewhen the whole remaining buffer was empty, never when the accurately-computeddegenerateflag (from the realsignerInfosSET) was true. No truncation needed to trigger this.Fix: one line, broadened the guard to also check
degenerate == 1:Tests added
test_wc_PKCS7_VerifySignedData_DegenerateMinimal-- minimal legit degenerate bundle still succeeds.test_wc_PKCS7_VerifySignedData_TruncSignerInfosTag/NoSignerInfosTag-- truncated/missingsignerInfosrejected.test_wc_PKCS7_VerifySignedData_DegenerateNonEmptyDigestAlgos-- bug 2 case returnsPKCS7_NO_SIGNER_E.NO_PKCS7_STREAM-only gate ontest_wc_PKCS7_VerifySignedData_TruncCertSetTag; it now passes in streaming mode too.Testing
Verified in isolated
git worktreebuilds (--enable-pkcs7 --enable-opensslextra, plusCFLAGS=-DNO_PKCS7_STREAMfor the non-streaming build), since the primary tree doesn't enable PKCS7. Each bug: negative test failed pre-fix, passed post-fix. No regressions inpkcs7_sd,wolfcrypt/test/testwolfcrypt, or the fulltests/unit.testsuite, in both build modes.